Thanks for the PR!

on memory constrained machines, the compiler gets killed trying to compile ext/fileinfo/data_file.c (via apprentice.c).

Looks like the behavior depends on the compiler: https://ci.appveyor.com/project/php/php-src/builds/46012603/job/mutvvhm5rg3jtnie#L1089.

Maybe a solution would be to add an option to create_data_file.php (which is not set by default) which creates the new format, and to let users run that script if they need it?

Looks like the behavior depends on the compiler

maybe combine both approaches?

@cmb69 and @mvorisek wrote:

Looks like the behavior depends on the compiler

maybe combine both approaches?

Yes, I saw AppVeyor's results on Windows :-(
So I agree we will have to combine both, because one way is a blocker on Windows, the other a blocker on 1 GB POSIX machines (hey, isn't Windows becoming a POSIX platform? … But I digress). As for 1 GB Windows build machines… nobody cares.

For this "two way approach", the ideal would be to have the original, 7 MB, magic.mgc, commited as the authoritative file,
and then both data_files generated at build; but I understand that not controlling the build env was the reason why the 42 MB generated data_file.c itself, and not the magic.mgc file, was commited.
In fact I couldn't find the magic.mgc: I suppose once in a while, a fileinfo maintainer extracts an "official" binary magic.mgc, runs create_data_file.php on it, commits the generated data_file.c but drops magic.mgc?

So, having an additional data_file_as_string.c (and not a replacement as I first committed) will require to commit either:

1. the binary magic.mgc as primary source, and then data_file.c and data_file_as_string.c, each generated at IC time by create_data_file.php magic.mgc
   (thank you @mvorisek for the pointer to the IC!)
2. or data_file.c as commited secondary source (no magic.mgc commited; create_data_file.php stays a maintainer's tool), and data_file_as_string.c generated at IC time by a translator from data_file.c (so an additional tool, because create_data_file.php only aims at simple binary files reading, without parsing)
3. … or only magic.mgc, and create data_file.c and data_file_as_string.c are generated at build time, by a create_data_file.c (C equivalent of the PHP one), which avoids a bootstrapping problem (not having PHP, or sed or awk or perl, on the build env) but adds a compiled tool.

I am always reluctant to commiting big and/or generated files, thus 3. would be the most optimal solution, but it is the most complex too.
If we stays in the current logic, then it would be 2. ("don't mind the megabytes, priority to ease of building").

@mvorisek, @cmb69, you seemed to agree on 2.?

Oldies someone?

After compiling both versions with all the old gccs and clangs I could find,
we can reasonably say that gcc and clang always prefer working on the string version.
Thus I am confident to choose the right file using a simple #if on the compiler.

(and yes I probably killed some polar bears :-( trying by dichotomy to find the minimal necessary memory for each combination. But this is for (unscientific) science… and saving subsequent polar bears by optimizing the millions of compilations of PHP still to occur)

Appendices

/* data_file_list.c: */
const unsigned char php_magic_database[7015032] = { 0x1C, 0x04, 0x1E, 0xF1, … };
/* data_file_long.c: */
/* /!\ Will require a conversion to unsigned char, works only if array size is multiple of 4, and DOES NOT WORK AS IS ON LITTLE ENDIAN PROCS (that is, all Intel ones): every long has to be reversed. */
const unsigned long php_magic_database[7015032/4] = { 0x1C041EF1, … };
/* data_file_string.c: */
const unsigned char php_magic_database[7015032] = "\x1C\x04\x1E\xF1…";
/* data_file_strings.c: */
const unsigned char php_magic_database[6851][1024] = {
"\x1C\x04\x1E\xF1…", /* 1024 bytes per string */
…, /* 6850 other lines. */
};

for CC in /usr/bin/clang /usr/local/gcc-*/bin/gcc /usr/local/clang-*/bin/clang
do
	v="`$CC --version | head -1`"
	for f in /tmp/data_file_list.c /tmp/data_file_long.c /tmp/data_file_string.c /tmp/data_file_strings.c
	do
		t=-
		de=0
		a=1048576
		while [ $a -gt $((de+1024)) ]
		do
			printf '\r\033[0K%s\t%s\t%s ' "$v" "$de" "$a"
			m=$(((de+a)/2))
			( ulimit -v $m ; /usr/bin/time -p $CC -c -o /tmp/1.o $f 2> /tmp/1.log >&2 ) \
			&& a=$m \
			&& t="`sed -e /^real/\!d -e 's#real[^0-9]*##g' < /tmp/1.log`" \
			|| de=$m
		done
		printf '\r\033[0K%d MB\t%s\t%s\t%s\n' $((a/1024)) "$t" "$f" "$v"
	done
done

On 3 different machines:

1024 MB -       /tmp/data_file_list.c   gcc34 (GCC) 3.4.6 20060404 (Red Hat 3.4.6-4.1)
736 MB  3.99    /tmp/data_file_long.c   gcc34 (GCC) 3.4.6 20060404 (Red Hat 3.4.6-4.1)
151 MB  2.48    /tmp/data_file_string.c gcc34 (GCC) 3.4.6 20060404 (Red Hat 3.4.6-4.1)
1024 MB -       /tmp/data_file_list.c   gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55)
334 MB  2.50    /tmp/data_file_long.c   gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55)
162 MB  2.50    /tmp/data_file_string.c gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55)
890 MB  14.12   /tmp/data_file_list.c   gcc (GCC) 7.5.0
356 MB  3.90    /tmp/data_file_long.c   gcc (GCC) 7.5.0
217 MB  2.89    /tmp/data_file_string.c gcc (GCC) 7.5.0

848 MB  18,21   /tmp/data_file_list.c   gcc (GCC) 7.3.0
311 MB  5,41    /tmp/data_file_long.c   gcc (GCC) 7.3.0
180 MB  4,91    /tmp/data_file_string.c gcc (GCC) 7.3.0
>180 MB  4,91    /tmp/data_file_strings.c gcc (GCC) 7.3.0
853 MB  17,36   /tmp/data_file_list.c   gcc (GCC) 7.4.0
315 MB  6,45    /tmp/data_file_long.c   gcc (GCC) 7.4.0
185 MB  4,77    /tmp/data_file_string.c gcc (GCC) 7.4.0
>185 MB  4,77    /tmp/data_file_strings.c gcc (GCC) 7.4.0
808 MB  8,92    /tmp/data_file_list.c   FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
299 MB  2,79    /tmp/data_file_long.c   FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
128 MB  0,39    /tmp/data_file_string.c FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
92 MB   0,37     /tmp/data_file_strings.c FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
1024 MB -       /tmp/data_file_list.c   clang version 4.0.1 (tags/RELEASE_401/final)
628 MB  4,24    /tmp/data_file_long.c   clang version 4.0.1 (tags/RELEASE_401/final)
476 MB  0,57    /tmp/data_file_string.c clang version 4.0.1 (tags/RELEASE_401/final)
440 MB  0,44    /tmp/data_file_strings.c clang version 4.0.1 (tags/RELEASE_401/final)
1024 MB -       /tmp/data_file_list.c   clang version 5.0.1 (tags/RELEASE_501/final)
632 MB  4,94    /tmp/data_file_long.c   clang version 5.0.1 (tags/RELEASE_501/final)
480 MB  0,57    /tmp/data_file_string.c clang version 5.0.1 (tags/RELEASE_501/final)
445 MB  0,45    /tmp/data_file_strings.c        clang version 5.0.1 (tags/RELEASE_501/final)
1024 MB -       /tmp/data_file_list.c   clang version 7.0.1 (tags/RELEASE_701/final)
644 MB  5,35    /tmp/data_file_long.c   clang version 7.0.1 (tags/RELEASE_701/final)
508 MB  0,61    /tmp/data_file_string.c clang version 7.0.1 (tags/RELEASE_701/final)
473 MB  0,49    /tmp/data_file_strings.c        clang version 7.0.1 (tags/RELEASE_701/final)
1024 MB -       /tmp/data_file_list.c   clang version 7.1.0 (tags/RELEASE_710/final)
647 MB  4,97    /tmp/data_file_long.c   clang version 7.1.0 (tags/RELEASE_710/final)
511 MB  0,60    /tmp/data_file_string.c clang version 7.1.0 (tags/RELEASE_710/final)
476 MB  0,50    /tmp/data_file_strings.c        clang version 7.1.0 (tags/RELEASE_710/final)
1024 MB -       /tmp/data_file_list.c   clang version 8.0.1 (tags/RELEASE_801/final)
669 MB  5,27    /tmp/data_file_long.c   clang version 8.0.1 (tags/RELEASE_801/final)
538 MB  0,65    /tmp/data_file_string.c clang version 8.0.1 (tags/RELEASE_801/final)
498 MB  0,55    /tmp/data_file_strings.c        clang version 8.0.1 (tags/RELEASE_801/final)

851 MB  11,96   /tmp/data_file_list.c   gcc (GCC) 11.2.0
303 MB  6,57    /tmp/data_file_long.c   gcc (GCC) 11.2.0
215 MB  3,47    /tmp/data_file_string.c gcc (GCC) 11.2.0
144 MB  14,00   /tmp/data_file_strings.c        gcc (GCC) 11.2.0
915 MB  5,79    /tmp/data_file_list.c   FreeBSD clang version 6.0.0 (tags/RELEASE_600/final 326565) (based on LLVM 6.0.0)
330 MB  3,83    /tmp/data_file_long.c   FreeBSD clang version 6.0.0 (tags/RELEASE_600/final 326565) (based on LLVM 6.0.0)
159 MB  0,33    /tmp/data_file_string.c FreeBSD clang version 6.0.0 (tags/RELEASE_600/final 326565) (based on LLVM 6.0.0)
109 MB  0,76    /tmp/data_file_strings.c        FreeBSD clang version 6.0.0 (tags/RELEASE_600/final 326565) (based on LLVM 6.0.0)
998 MB  4,77    /tmp/data_file_list.c   clang version 12.0.1
393 MB  4,94    /tmp/data_file_long.c   clang version 12.0.1
247 MB  0,26    /tmp/data_file_string.c clang version 12.0.1
198 MB  1,60    /tmp/data_file_strings.c        clang version 12.0.1
975 MB  5,52    /tmp/data_file_list.c   clang version 13.0.0
370 MB  4,32    /tmp/data_file_long.c   clang version 13.0.0
225 MB  0,41    /tmp/data_file_string.c clang version 13.0.0
175 MB  0,53    /tmp/data_file_strings.c        clang version 13.0.0

82 MB   9,11    /tmp/data_file_strings.c        gcc (Raspbian 8.3.0-6+rpi1) 8.3.0

/!\ Do not compare between successive versions of a same compiler: some are stock ones, some compiled by myself, some are monolithic and some dynamically linked, and so on. I could even have forgot -O3 to create some of them.

… And I found after having run the whole loops for hours, that my tests were unequal, because data_file_list.c had an array of 7015032 bytes, while data_file_string.c had one of 7955032.
Thus the comparison was (a bit) unfair to data_file_string.c.

So I opted for solution 2. (the one that seemed the most in the spirit of the current ext).

@mvorisek, @cmb69, does this seem right to you?

this might increase the sources size substantially, so I would prefer an unified solution

did you tried something like { 0x1C041EF1, 0x1C041EF1, … };, ie. larger value in the array initialization (of course with correct typecast), ot something like { "\x...", "\x...", ... }

this might increase the sources size substantially, so I would prefer an unified solution

Yes, I was wondering why in the first place we had this big 44 MB data_file.c in the repository instead of its 7 MB binary source. Do you know the reason?
I supposed:

• binaries are not welcome?
  (but the only advantage I see, benefitting from git's hability to patch only part of a text file, is defeated if the source magic.mgc offsets its contents to include new rules: this would shift all contents in the derived data_file.c, and prevent git from diffing it. In fact when git diffing the last commits that updated data_file.c, git tells me that "Binary files ext/fileinfo/data_file.c and ext/fileinfo/data_file.c differ": hint that it treats not differently from a binary)
• or we have no simple way to generate data_file.c at compile time?
  (particularly on Windows, where we cannot rely on any POSIX tool), so it was chosen to push the pregenerated file

This is why I thought proposal 3. was the cleanest one.

did you tried something like { 0x1C041EF1, 0x1C041EF1, … };, ie. larger value in the array initialization (of course with correct typecast), ot something like { "\x...", "\x...", ... }

First form would cause problems with endianness (we would have to provide two files, or one file with macros around each block: that would defeat the goal of reducing its size),
and second one seems to combine both list and string, so I fear that it will combine their disadvantages too :-(

I still highly prefer an unified solution and I belive it is possible to achive it. Also, when you look into the source file data, there are many 0x00 bytes, maybe some compression like run-length encoding would solve this problem much better and reduce the binary size of the lib.

maybe some compression like run-length encoding would solve this problem much better and reduce the binary size of the lib.

… But afterwards I'm thinking that every installation of PHP since its beginnings (?) uses the flat version,
so maybe everyone prefers to sacrifice size to speed, and compressing it would be seen as a regression? (or maybe nobody cared… Particularly as the table grow slowly from 1 MB?)

Anyway, I wouldn't pretend modifying the runtime inner workings of ext/fileinfo, I was just here to solve a compile-time problem :-) so it's a bit beyond my goal (and skills).

OK, now with cb44e9c I think it's much better:

• no commit of a big file
• no modification on the runtime (still the big-but-quick flat array)
• no modification on the CI time
• the big file gets generated at compile time
  only on Makefile platforms (thus, not on Windows),
  with a fallback to include data_file.c (as in the current master),
  and is included only after strict conditions were verified (GCC or Clang; and only if non-Windows. Because yes the build guidelines tell that "experimentally, clang can be used on Windows", so we have to fallback if any of the conditions is true).

The other good news is that I found mentions to minilua, and I begin to understand how to have a build-time utility compiled from C:
thus if the sed used to create data_file_string.c from data_file.c is too slow, I can always reimplement it as C.

@mvorisek, would you mind re-running the checks? Github insists on using the forelast version of actions.yml, although I discarded it in the last version :-(

The data file/var is loaded in ext/fileinfo/libmagic/apprentice.c only, so the variable can be declared with any content - { 0x1C041EF1, 0x1C041EF1, … } solution should be easily possible at least

The data file/var is loaded in ext/fileinfo/libmagic/apprentice.c only, so the variable can be declared with any content - { 0x1C041EF1, 0x1C041EF1, … } solution should be easily possible at least

Yes, you were right, using longs gives a performance boost (see my updated #10422 (comment)).

Notes:

• this was just to test compilation consumption, not viability. An adaptation will be required to make apprentice.c use a long array instead of a char one.
  The good news is that libmagic internally works with longs, thus using longs removes one layer of transcription (long -> char -> long). And apprentice.c graciously handles both endianness (see variable needsbyteswap), so the most that we can mess by choosing the wrong endianness, is a performance penalty at startup of the lib when it has to byte-swap 7 MB of data (only once).
  Perhaps we can do the effort of ordering the file in the target's endianness at compile time, to avoid the library back-byte-swaping it at runtime. But if we fail that's not blocking, just a processor cycles waste.
• memory-wise and CPU-wise (which was the goal of the P.R.) we're still not as efficient as a big string.
  GCC: 1.5x - 2x hungrier / slower than string
  clang: 1.5x - 2x hungrier, and 10x slower, than string

So I see it as a (quite acceptable) compromise,
but that requires some more work (on apprentice.c and create_data_file.php) before being usable.

@mvorisek that might work, a multidimensional array is still contiguous. If it fixes @outtersg's issue, too, would be great :)

I totally agree (I 👍 ed @mvorisek).

My only concern (to test now) is on padding:
I checked on data_file.c's history, the longest common denominator is… 8 bytes. Thus if we want 1024 bytes strings, there will be padding.
gcc and clang have no problem having the last string incomplete (they pad with nul chars, without warning),
for MSVC I'll let AppVeyor test.
But I'll have to check that there is no sizeof(php_magic_database): it will include padding, so either libmagic's state machine doesn't mind having that at the end, or we'll have to replace the sizeof with something more accurate.

Woooh, I didn't expected it… but your hybrid "list of strings" solution @mvorisek was even better (for GCC and Clang: less memory, quicker to compile) than the "one pure string" one!
It seems that 1024 is the right balance between string length and array count.

@weltling wrote:

A system with limited resources will fail if not here then at another place.

I feared it too, but with this patch the brave RPi3 finally succeeded into compiling a full PHP 8.2.1 in 90 mn (make -j1 to avoid crashing it again). Yay!

@outtersg perfect! Did you tested compilation with all exts enabled? What are the next bottlenecks, can we fix more of them? (and possibly reduce the total compile time too)

Did you tested compilation with all exts enabled?

No, this is a tuned-for-my-use installation:

./configure \
    '--with-zlib' '--with-iconv' '--enable-exif' '--with-gd' '--with-jpeg-dir' \
    '--with-readline' '--with-curl' '--enable-fileinfo=shared' '--enable-shared' \
    '--enable-mbstring' '--enable-soap' '--enable-sysvsem' '--enable-sysvshm' \
    '--with-gettext' '--with-openssl' '--enable-zip' '--enable-sockets' \
    '--with-pgsql' '--with-pdo-pgsql' '--without-mysql' '--without-pdo-mysql' \
    '--enable-fpm' '--enable-intl' '--enable-phpdbg' '--with-freetype' \
    '--enable-calendar' \
    '--with-sqlite3=/home/pi/local/sqlite-3.40.1' '--with-pdo-sqlite=/home/pi/local/sqlite-3.40.1'

What are the next bottlenecks, can we fix more of them? (and possibly reduce the total compile time too)

I could not find any other pathological .c file (C files over 10 MB); this one was really a corner case for multiple compilers.
I feel that, even if there were other compiler optimization opportunities, they wouldn't be worth the effort, because the gain would be unnoticeable.

@mvorisek @outtersg very nice solution. The memory overhead i'd consider minimal and the current approach is at base not changed. I'd suggest to give it a try in master. One can then observe on further bug fixes and libmagic upgrades.

Thanks!

OK, now that #10533 has been pushed, correcting the CI for master,
is there a way to force the CI to reconsider my last commit?

@weltling wrote:

close and reopen PR has worked in the past to re-trigger the CI, maybe worth trying.

… aaaaand wait for master to be CI-stable! OK, now that 18b611d on master made it pass, @mvorisek, we're good to go!

@outtersg close and reopen PR has worked in the past to re-trigger the CI, maybe worth trying.

Thanks

@mvorisek, may you find a bit of time to finish this PR, now that we refined it (and successfully fought against the CI chain!)?

Thank you, as well as @weltling,
and a nice Easter to both of you!

LGTM, maybe the (original) size should declared in C explicitly, as the NUL appended bytes might be not be expected.

@iluuu1994 can you please review?

LGTM, maybe the (original) size should declared in C explicitly, as the NUL appended bytes might be not be expected.

@iluuu1994 can you please review?

Looking at apprentice.c, the only use path of php_magic_database is with a cast to struct magic * (through map->p = php_magic_database);
and all sizeof are done on struct magic instead of php_magic_database.

Thus the padding's size as well as its contents has zero incidence on the runtime (… hey, thus I could put an ode to myself in it!), apart from the less-than-1 KB waste.
the only requirement is that php_magic_database is at least the size of struct magic.

In addition to fixing the build with the newly pushed data_file.c (#13369),
I switched to octal + plain strings which:

• improves readability (ASCII chunks appear literally)
• without increasing complexity (but beware I tested on one config only, where there was an insignificant gain; it would be interesting having a mem + time test on Windows and a Raspberry Pi).

I put this on my TODO to take a look at. Sorry it took so long to find a reviewer.